Skip to content

fix(temporal): retry Glitter startup metric restoration - #2006

Open
shepherdjerred wants to merge 1 commit into
mainfrom
glitter-pagerduty-alert
Open

fix(temporal): retry Glitter startup metric restoration#2006
shepherdjerred wants to merge 1 commit into
mainfrom
glitter-pagerduty-alert

Conversation

@shepherdjerred

Copy link
Copy Markdown
Owner

Summary

  • Add shutdown-aware indefinite equal-jitter retries for Glitter startup snapshot metric restoration.
  • Retry transient SeaweedFS connection and 408/429/5xx failures while preserving alertable permanent failures.
  • Log retries and escalate to Sentry after 10 consecutive transient failures.
  • Add focused retry and storage-classification tests.

Verification

  • bun test src/shared src/activities: 337 passed
  • bun run typecheck: passed
  • Changed-file ESLint and Prettier: passed
  • Full lint still reports unrelated existing repository violations.

The Prometheus alert expression, schedule timing, and PagerDuty routing are unchanged.

@shepherdjerred

Copy link
Copy Markdown
Owner Author

This change is part of the following stack:

Change managed by git-spice.

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 8, 2026

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

New Review Started

This review has been superseded by a new analysis

Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 8, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Retry Glitter startup snapshot-metric restoration on transient storage failures

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add shutdown-aware equal-jitter retries for Glitter startup snapshot metric restoration.
• Retry only transient SeaweedFS/HTTP failures; escalate to Sentry after 10 consecutive failures.
• Add tests for retry delays, shutdown cancellation, and transient storage error classification.
Diagram

graph TD
  A["Temporal worker"] --> B["retryUntilReady"] --> C["Restore snapshot metrics"] --> D[("SeaweedFS / S3")]
  B --> E["Structured logger"]
  B --> F{{"Sentry"}}

  subgraph Legend
    direction LR
    _svc["Service/module"] ~~~ _db[("Storage")] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Mitigate via alert tuning (startup grace window)
  • ➕ No code changes; reduces operational risk of retry loops
  • ➕ Keeps worker startup simpler
  • ➖ Weakens the alert signal for real snapshot issues during the grace period
  • ➖ Does not address other startup consumers of SeaweedFS; pushes logic into monitoring
2. Infrastructure gating (init container / readiness on SeaweedFS)
  • ➕ Prevents the race for all startup dependencies, not just metrics restoration
  • ➕ Keeps application logic simpler
  • ➖ Harder to express reliably across environments and failure modes
  • ➖ May delay all worker readiness even if only corpus storage is impacted
3. Reuse existing supervisor pattern (linear backoff like event-bridge)
  • ➕ Consistency with existing worker supervision approach
  • ➕ Less new utility surface area
  • ➖ Less robust than equal-jitter exponential backoff under contention
  • ➖ Harder to unit-test deterministically without extracting a shared helper anyway

Recommendation: Proceed with the PR’s approach: a small, testable retry supervisor with explicit transient/permanent classification preserves alert semantics while eliminating known SeaweedFS startup races. The utility is generic enough to reuse (similar to the event-bridge supervisor), but remains scoped and shutdown-aware to avoid masking real failures.

Files changed (6) +361 / -15

Enhancement (1) +112 / -0
startup-retry.tsAdd shutdown-aware equal-jitter exponential backoff retry helper +112/-0

Add shutdown-aware equal-jitter exponential backoff retry helper

• Introduces 'retryUntilReady' plus 'equalJitterRetryDelayMs' and 'sleepUnlessClosed' to support indefinite startup retries with exponential ceiling and equal-jitter delays. Supports dependency injection for sleep/random and optional one-time escalation callback.

packages/temporal/src/shared/startup-retry.ts

Bug fix (2) +51 / -14
glitter-corpus-store.tsClassify transient corpus storage failures for safe retries +20/-0

Classify transient corpus storage failures for safe retries

• Introduces 'isTransientCorpusStorageError' to detect retryable failures via HTTP status codes (408/429/5xx) and common network error strings. Keeps existing not-found and precondition classification intact.

packages/temporal/src/activities/glitter-corpus-store.ts

worker.tsWrap Glitter startup metric restoration in transient-only retry loop +31/-14

Wrap Glitter startup metric restoration in transient-only retry loop

• Replaces the one-shot startup snapshot-metric restoration with 'retryUntilReady', retrying only when 'isTransientCorpusStorageError' matches. Logs every retry attempt, emits a single Sentry warning after 10 consecutive transient failures, and stops retrying once shutdown begins.

packages/temporal/src/worker.ts

Tests (2) +163 / -1
glitter-corpus-storage.test.tsAdd tests for transient SeaweedFS/S3 error classification +39/-1

Add tests for transient SeaweedFS/S3 error classification

• Imports and exercises the new transient error classifier across common connection error codes and retryable HTTP status codes. Verifies that permanent errors (auth/404 and malformed data) are not treated as retryable.

packages/temporal/src/activities/glitter-corpus-storage.test.ts

startup-retry.test.tsAdd deterministic tests for equal-jitter startup retry supervisor +124/-0

Add deterministic tests for equal-jitter startup retry supervisor

• Adds unit tests for equal-jitter delay calculation, delay capping, retry-until-success flow, shutdown cancellation, and single escalation behavior at the 10th consecutive transient failure.

packages/temporal/src/shared/startup-retry.test.ts

Documentation (1) +35 / -0
2026-08-07_glitter-corpus-startup-metric-retry.mdDocument plan for self-healing Glitter startup metric restoration +35/-0

Document plan for self-healing Glitter startup metric restoration

• Adds an in-progress plan describing indefinite equal-jitter retries for startup snapshot-metric restoration. Documents transient vs permanent failure handling, shutdown behavior, and verification steps.

packages/docs/plans/2026-08-07_glitter-corpus-startup-metric-retry.md

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1a2694e6f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +37 to +40
return (
error instanceof Error &&
TRANSIENT_STORAGE_ERROR_PATTERN.test(`${error.name} ${error.message}`)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Read the structured transport error code

When SeaweedFS resets a connection, the Node/Bun HTTP stack can surface an error such as Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }); the code is not guaranteed to appear in name or message. This predicate therefore returns false for a transient failure it explicitly intends to handle, causing the startup supervisor to stop after the first failed restoration and leaving the snapshot metric absent until the worker restarts. Parse and inspect the structured code field (and, where applicable, the transport cause) rather than testing only rendered error text.

Useful? React with 👍 / 👎.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Cause chain not retried 🐞 Bug ☼ Reliability
Description
isTransientCorpusStorageError only checks the top-level error’s $metadata.httpStatusCode and a regex
against ${error.name} ${error.message}, so a transient connection failure that is wrapped in an
Error.cause chain (where the outer error message doesn’t include the connection code) will be
treated as non-transient and will stop the startup retry loop.
Code

packages/temporal/src/activities/glitter-corpus-store.ts[R37-40]

+  return (
+    error instanceof Error &&
+    TRANSIENT_STORAGE_ERROR_PATTERN.test(`${error.name} ${error.message}`)
+  );
Evidence
The new classifier only matches transient connection codes in the outer Error’s name/message and
does not inspect .cause, while other code in this repo explicitly traverses .cause chains to
find the real underlying failure signal—meaning transient network failures can be hidden by wrappers
and incorrectly classified as non-transient.

packages/temporal/src/activities/glitter-corpus-store.ts[18-41]
packages/temporal/src/activities/data-dragon-util.ts[209-226]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`isTransientCorpusStorageError` ignores `Error.cause` and only inspects the outer error’s `name/message` for connection codes. If an SDK/network layer wraps the underlying connection error (common pattern in this repo), the transient signal may live in `cause` and retries won’t happen.

## Issue Context
There is already repo precedent for walking `.cause` chains to find the real failure message.

## Fix Focus Areas
- packages/temporal/src/activities/glitter-corpus-store.ts[23-41]
- packages/temporal/src/activities/data-dragon-util.ts[217-226]

## Suggested fix
- Build a helper to iterate `error` and its `.cause` chain (with cycle protection / depth limit), and for each link:
 - check `$metadata.httpStatusCode` (408/429/5xx)
 - check connection code patterns against combined text (e.g., `${name} ${message} ${stack ?? ""}`)
 - optionally check a common `code` field (e.g., `(err as any).code`) if present.
- Return true if any link matches transient criteria.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Misleading completion log 🐞 Bug ◔ Observability
Description
worker.ts logs “Glitter corpus snapshot metric restoration completed” whenever retryUntilReady
returns "succeeded", but restoreGlitterCorpusSnapshotMetrics can return early without restoring
anything (not configured or latest pointer missing), making the log message inaccurate.
Code

packages/temporal/src/worker.ts[R117-119]

+    if (result === "succeeded") {
+      jsonLog("info", "Glitter corpus snapshot metric restoration completed");
+    }
Evidence
The worker logs completion when the retry wrapper reports success, but the restoration function
explicitly returns early in multiple cases where no restoration occurs, so the new log message
overstates what happened.

packages/temporal/src/worker.ts[87-120]
packages/temporal/src/activities/glitter-corpus-snapshot.ts[41-59]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The worker logs a completion message on any successful return from `restoreGlitterCorpusSnapshotMetrics`, but that function can legitimately do no work (e.g., metrics not configured or pointer missing). This makes logs less trustworthy during incident response.

## Issue Context
`restoreGlitterCorpusSnapshotMetrics` returns `void` and uses early returns for “skip” cases.

## Fix Focus Areas
- packages/temporal/src/worker.ts[91-119]
- packages/temporal/src/activities/glitter-corpus-snapshot.ts[41-59]

## Suggested fix
- Change `restoreGlitterCorpusSnapshotMetrics` to return a small status enum, e.g. `"restored" | "not_configured" | "pointer_missing"`.
- In `worker.ts`, log different messages (or include a field like `{ outcome }`) and only say “completed”/“restored” when metrics were actually updated.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context used
✅ Compliance rules (platform): 52 rules

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment on lines +37 to +40
return (
error instanceof Error &&
TRANSIENT_STORAGE_ERROR_PATTERN.test(`${error.name} ${error.message}`)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Cause chain not retried 🐞 Bug ☼ Reliability

isTransientCorpusStorageError only checks the top-level error’s $metadata.httpStatusCode and a regex
against ${error.name} ${error.message}, so a transient connection failure that is wrapped in an
Error.cause chain (where the outer error message doesn’t include the connection code) will be
treated as non-transient and will stop the startup retry loop.
Agent Prompt
## Issue description
`isTransientCorpusStorageError` ignores `Error.cause` and only inspects the outer error’s `name/message` for connection codes. If an SDK/network layer wraps the underlying connection error (common pattern in this repo), the transient signal may live in `cause` and retries won’t happen.

## Issue Context
There is already repo precedent for walking `.cause` chains to find the real failure message.

## Fix Focus Areas
- packages/temporal/src/activities/glitter-corpus-store.ts[23-41]
- packages/temporal/src/activities/data-dragon-util.ts[217-226]

## Suggested fix
- Build a helper to iterate `error` and its `.cause` chain (with cycle protection / depth limit), and for each link:
  - check `$metadata.httpStatusCode` (408/429/5xx)
  - check connection code patterns against combined text (e.g., `${name} ${message} ${stack ?? ""}`)
  - optionally check a common `code` field (e.g., `(err as any).code`) if present.
- Return true if any link matches transient criteria.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +117 to +119
if (result === "succeeded") {
jsonLog("info", "Glitter corpus snapshot metric restoration completed");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

2. Misleading completion log 🐞 Bug ◔ Observability

worker.ts logs “Glitter corpus snapshot metric restoration completed” whenever retryUntilReady
returns "succeeded", but restoreGlitterCorpusSnapshotMetrics can return early without restoring
anything (not configured or latest pointer missing), making the log message inaccurate.
Agent Prompt
## Issue description
The worker logs a completion message on any successful return from `restoreGlitterCorpusSnapshotMetrics`, but that function can legitimately do no work (e.g., metrics not configured or pointer missing). This makes logs less trustworthy during incident response.

## Issue Context
`restoreGlitterCorpusSnapshotMetrics` returns `void` and uses early returns for “skip” cases.

## Fix Focus Areas
- packages/temporal/src/worker.ts[91-119]
- packages/temporal/src/activities/glitter-corpus-snapshot.ts[41-59]

## Suggested fix
- Change `restoreGlitterCorpusSnapshotMetrics` to return a small status enum, e.g. `"restored" | "not_configured" | "pointer_missing"`.
- In `worker.ts`, log different messages (or include a field like `{ outcome }`) and only say “completed”/“restored” when metrics were actually updated.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant